Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for save_method PATCH #483

Merged
merged 4 commits into from
Jul 12, 2023

Conversation

thilak009
Copy link
Contributor

@thilak009 thilak009 commented Jul 3, 2023

Changes proposed

Add support for PATCH as a save_method, currently only PUT is supported as a save_method

  • use JSONPatchAction to validate and read JSON patch operations
  • add patch_policy_data to opal_client which does a PATCH API call to OPA
    • based on the save_method value, call either set_policy_data or patch_policy_data respectively
  • added jsonpatch package to apply JSON patch operations on OPA data cache when offline mode is enabled
  • added a function custom_encoder for using it as the default encoder for doing json.dumps on the Union type JsonableValue to ignore default fields values and dump based on alias name

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Note to reviewers

Need thoughts/help on if there is a better way to do custom_encoder implementation, haven't used pydantic before

the reason i used it was because FastAPI does by default print response using JSON alias names but we are not returning response from FastAPI server but rather feeding the model directly to OPA in which case the field name would be used when doing a json.dumps instead of alias name

and also to remove default None value populated fields from the dumped JSON for any request that matches the JSONPatchAction type

One more thing is, OPA does not support move operation as a JSON patch operation, refer discussion and hence OPAL would also not support it
what would be a good place in the docs to mentions this as a NOTE ?

@netlify
Copy link

netlify bot commented Jul 3, 2023

Deploy Preview for opal-docs canceled.

Name Link
🔨 Latest commit f2899a1
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/64ac4e7f065fff000844b2e3

@orweis
Copy link
Contributor

orweis commented Jul 3, 2023

One more thing is, OPA does not support move operation as a JSON patch operation, refer discussion and hence OPAL would also not support it what would be a good place in the docs to mentions this as a NOTE ?

I think this can go together with the full docs for this feature as a section in the data-updates tutorial and perhaps a quick reference in the data-sources article

@orweis orweis requested a review from roekatz July 4, 2023 07:40
Copy link
Collaborator

@roekatz roekatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thilak009,
Seems like a very solid PR to me, Well done!
Had one or two comments/question - please fix/reply and we'll go on to merge that.

I liked your solution to OpaClient's data cache. Unfortunately we don't have unit tests for it - have you tried (manually) testing the backup feature? (OFFLINE_MODE).

Thank you! your contribution is much appreciated

@thilak009
Copy link
Contributor Author

thilak009 commented Jul 10, 2023

I liked your solution to OpaClient's data cache. Unfortunately we don't have unit tests for it - have you tried (manually) testing the backup feature? (OFFLINE_MODE).

yep, i checked the updates in the JSON file manually when OFFLINE_MODE is enabled

@thilak009
Copy link
Contributor Author

also as Or Weis mentioned, adding this comment
I am authorizing this contribution to open source

@thilak009 thilak009 requested a review from roekatz July 11, 2023 18:55
Copy link
Collaborator

@roekatz roekatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @thilak009 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants